fix(security): stop a project's CSRF policy blocking its own channel dispatch - #3647
Conversation
…dispatch PR #3641 fixed the control-plane routes. The same class of failure survives on `POST /channels/invoke`. A Slack or Discord message reaches an agent because the platform channel dispatcher POSTs `/channels/invoke` carrying a signed dispatch envelope in `x-veryfront-dispatch-jws`, and `resolveRuntimeOwnerInvokeUrl` re-dispatches to the same route when another runtime instance owns the run. Neither caller is a browser, so neither holds a `__Host-vf_csrf` cookie to echo. `CsrfHandler` runs at priority 5 with an empty pattern list, ahead of `ChannelInvokeHandler` at 700, so any project that set `security.csrf` to anything truthy answered its own channel dispatch with `403 Forbidden - invalid or missing CSRF token`. The agent never ran and the channel just went quiet: nothing in the failure names CSRF or config. Scoping. `/channels/invoke` is NOT a control-plane surface and must not be added to `isControlPlaneSurfaceRoute`. The `CONTROL_PLANE_SURFACES` enumeration `["studio","channels","a2a","mcp"]` is the `surface` claim inside a control-plane envelope -- it names which product surface an agents-list request is made for, not this HTTP route. The route carries a different envelope entirely: `verifyDispatchJws` against `DispatchClaims`, not `verifyControlPlaneJws` against `ControlPlaneClaims`, under a different header. Extending the existing shape list would therefore have been wrong twice over: the caller does not present the control-plane header, so the entry would do nothing, and making it do something would mean loosening the header check that makes `isSignedControlPlaneDispatch` narrow. So it gets its own positively-identified, proof-carrying predicate rather than a path exemption. `isSignedChannelDispatch` returns true only when BOTH the request is exactly `POST /channels/invoke` -- matched against `URL.pathname`, which resolves dot segments, and never by prefix, because `/channels/` is reserved but not exclusively routed -- AND it carries `x-veryfront-dispatch-jws`. `verifyDispatchJws` then binds the Ed25519 signature to the issuer, project audience, project id, dispatch id, platform and a SHA-256 hash of the body with expiry and skew bounds, and the handler rejects an envelope whose claims disagree with the payload it acts on. The exemption therefore never covers anything that is not authenticated more strongly downstream. The proxy classifier drops its second copy of the same route shape and consumes `isChannelDispatchRoute`, the way #3641 folded the control-plane table together. Covered by a dispatch test that drives the real chain -- `CsrfHandler` then `ChannelInvokeHandler`, with a real Ed25519-signed envelope -- across the csrf shapes, and asserts the agent is reached. The adversarial cases assert the gate still returns 403 for: an invoke POST with no signature, an invoke POST carrying only a control-plane signature, a dispatch signature on a control-plane surface, an empty signature header, and a genuinely signed envelope aimed at `/channels/invoke/application-route`, `/channels/invoker`, `/channels/invoke-mirror/run`, `/channels`, `/api/channels/invoke`, `PUT` and `DELETE`.
📝 WalkthroughWalkthrough
ChangesChannel dispatch security
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant CsrfHandler
participant ChannelInvokeHandler
participant Agent
Request->>CsrfHandler: POST /channels/invoke with dispatch JWS
CsrfHandler->>CsrfHandler: classify signed channel dispatch
CsrfHandler->>ChannelInvokeHandler: bypass CSRF validation
ChannelInvokeHandler->>Agent: dispatch channel request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/channels/invoke-dispatch-security.test.ts`:
- Around line 245-263: Add an integration test alongside the existing invoke
POST rejection cases that passes a malformed x-veryfront-dispatch-jws value
through dispatchChannelInvoke, exercising CsrfHandler and RouteRegistry. Assert
that the full request chain returns status 401 and answered === false.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f21eace7-d3d6-418b-937f-819de0f86f63
📒 Files selected for processing (6)
docs/api-reference/veryfront/security.mdsrc/channels/control-plane.tssrc/channels/invoke-dispatch-security.test.tssrc/proxy/control-plane-signature.tssrc/security/http/csrf/csrf-handler.test.tssrc/security/http/csrf/csrf-handler.ts
Two follow-ups from reading this alongside #3641, #3645 and #3646Both are handled in #3649, which stacks on this branch — nothing needs to change here. 1. The exemption is only one gate out of three. On the merged tree, a channel dispatch carrying its envelope was still 401'd by a project's 2. The docstring inherited from #3641 is unsound.
The runtime grants it. With no configured The header is attacker-settable. #3649 corrects both copies to state the real basis — the exemption skips only the browser-credential gate, authority comes from the downstream verification an attacker cannot forge, and every admitted route terminates at a handler doing that check ahead of The corrections are in #3649 rather than here so both copies of the wording land together. |
…cted The CSRF exemption is granted on the dispatch header being *present*, so all of its safety rests on `ChannelInvokeHandler` verifying the envelope behind it. That downstream rejection had no end-to-end coverage: the full-chain test only ever minted valid envelopes, and the malformed-signature cases in `csrf-handler.test.ts` exercise `CsrfHandler` alone, never `RouteRegistry` or the invoke handler. Drives the real chain with a request that *takes* the exemption and must still fail: `CsrfHandler` steps aside (so the answer is not 403), the handler fails `verifyDispatchJws`, and the runtime answers 401 with the agent never reached. Both failure modes are covered -- a value that is not a compact JWS, and a well-formed envelope correctly bound to this body but signed by a key the runtime does not trust.
…t just CSRF #3647 exempted a signed channel dispatch from CSRF. On the merged tree the same dispatch was still 401'd by a project's `security.auth` and still turned away by a gating root `middleware.ts`, so the caller that fix set out to unblock was still blocked twice, by the two gates nobody had reason to look at. Apply `isSignedChannelDispatch` at those two sites as well. The two predicate families stay separate on purpose: a channel dispatch carries a different envelope under a different header, verified by `verifyDispatchJws` against the dispatch id, platform, project id and body hash rather than by `verifyControlPlaneJws` against a method and path. But the set of gates each is exempt from must be the same, and that is now the thing under test. Add the matrix: {control-plane dispatch, channel dispatch} x {security.auth, security.csrf, project middleware.ts}, each cell asserting admitted when validly signed, rejected at the gate when unsigned, and rejected downstream when forged. The forged column carries the real security argument: the signature header is attacker-settable, so what makes an exemption safe is that the route terminates at a handler that verifies the envelope, never at project code. The file is the artifact that stops the next gate from landing with a partial exemption. The middleware guide gains the consequence a project author needs: root middleware does not run for the project's own channel traffic either.
#3641's docstring, repeated verbatim by #3647, argued the exemption was safe because "a browser cannot attach the signature header to a cross-origin request without a preflight the runtime does not grant". The runtime does grant it: with no configured `allowedHeaders`, `resolveNormalizedCORSPreflightPolicy` reflects whatever `Access-Control-Request-Headers` asked for, so any project whose CORS policy admits an origin advertises the signature header to it. The proxy also forwards an unverified `x-veryfront-*-jws` from a public request rather than stripping it. State the actual basis instead: the exemption skips only the browser-credential gate, authority still comes from the downstream signature verification that an attacker cannot forge, and every admitted route terminates at a handler doing that verification ahead of ApiHandlerWrapper. Same correction to the three gate comments that said the exemption is keyed on "the request being a real dispatch": no predicate can know that from a header.
The hole
PR #3641 fixed the control-plane routes. The same class of failure survives on a different path:
POST /channels/invoke.A Slack or Discord message reaches an agent because the platform channel dispatcher POSTs
/channels/invokecarrying a signed dispatch envelope inx-veryfront-dispatch-jws, andresolveRuntimeOwnerInvokeUrl(src/internal-agents/runtime-owner.ts) re-dispatches to the same route when another runtime instance owns the run. Neither caller is a browser, so neither holds a__Host-vf_csrfcookie to echo.CsrfHandlerruns at priority 5 withpatterns: [];ChannelInvokeHandlerruns at 700. So any project that setsecurity.csrfto anything truthy answered its own channel dispatch with403 Forbidden - invalid or missing CSRF token. The agent never ran and the channel just went quiet -- nothing in the failure names CSRF or config.Scoping: is this a control-plane surface?
No, and it must not be added to
isControlPlaneSurfaceRoute.CONTROL_PLANE_SURFACES = ["studio","channels","a2a","mcp"]is thesurfaceclaim inside a control-plane envelope -- it names which product surface an agents-list request is made for. It is not a route table, and its"channels"entry is not the/channels/invokeHTTP route.The route carries a different envelope entirely:
/channels/invokex-veryfront-control-plane-jwsx-veryfront-dispatch-jwsverifyControlPlaneJwsverifyDispatchJwsControlPlaneClaims(surface, request_method, request_path, request_hash)DispatchClaims(platform, sub=dispatchId, body_sha256)Extending the existing shape list would have been wrong twice over: the caller does not present the control-plane header, so the new entry would do nothing; and making it do something would mean loosening the header check that is exactly what keeps
isSignedControlPlaneDispatchnarrow.The proxy layer already models this distinction --
classifyInternalControlPlaneRequestreturns"dispatch", not"control-plane", for this route, andsrc/proxy/control-plane-signature.test.tsalready asserts "does not accept a control-plane JWS on /channels/invoke".The fix -- same shape as #3641, second mechanism justified
isSignedChannelDispatch(req)returns true only when both:isChannelDispatchRoutematches -- exactlyPOST /channels/invoke, matched againstURL.pathname(which resolves dot segments), never by prefix, because/channels/is reserved but not exclusively routed; andx-veryfront-dispatch-jws.It is not a weakening: it exempts nothing that is not authenticated more strongly downstream.
verifyDispatchJwsbinds the Ed25519 signature to issuer, project audience, project id, dispatch id, platform and a SHA-256 hash of the body, with expiry and skew bounds;readSignedChannelDispatchRequestthen rejects any envelope whose claims disagree with the payload it acts on.The proxy classifier drops its second copy of the same route shape and consumes
isChannelDispatchRoute, the way #3641 folded the control-plane table together.Tests
RED first.
src/channels/invoke-dispatch-security.test.tsdrives the real chain --RouteRegistrywithCsrfHandlerthenChannelInvokeHandler, a real Ed25519-signed envelope -- and failed for the right reason before the fix:The three adversarial cases in that file passed before and after.
Adversarial coverage (the gate must still reject). 403 is asserted for:
401and the agent never reached/channels/invoke/application-route,/channels/invoker,/channels/invoke-mirror/run,/channels,/api/channels/invoke, andPUT/DELETEon the real pathNo existing test was weakened or deleted.
docs/api-reference/veryfront/security.mdis a regenerated line pin (deno task docs), not a hand edit.Verification
Deno 2.7.7.
deno check,deno fmt:check,deno lint,lint:module-boundaries,lint:dependency-boundaries,lint:barrel-jsdoc,docs:api-reference:checkall clean.deno test src/proxy/ src/channels/ src/security/ src/server/handlers/request/-> 199 passed / 1 failed. The one failure isagent-stream.handler.test.ts, confirmed pre-existing: an identical run on a detachedorigin/mainworktree produces a byte-identical failing-step set (diff-> IDENTICAL FAILING SET).Summary by CodeRabbit
New Features
POST /channels/invokerequests from similar or unsupported routes.Bug Fixes
Documentation
CsrfHandlersource reference in the security API documentation.